fix: unique popup missing className #570
Conversation
Walkthrough本次变更在 UniqueProvider 中引入基于对齐信息的弹层 className 计算；在上下文选项中新增可选回调 getPopupClassNameFromAlign；在入口处将该回调透传到 UniqueProvider；并新增对应的单测覆盖该行为。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as 用户
participant Trg as Trigger
participant UP as UniqueProvider
participant Pop as Popup
User->>Trg: 点击目标元素
Trg->>UP: show(options{ builtinPlacements, prefixCls, getPopupClassNameFromAlign })
UP->>UP: 计算 alignInfo（基于触发位置）
UP->>UP: alignedClassName = classNames( getAlignPopupClassName(...), options.getPopupClassNameFromAlign?.(alignInfo) )
UP->>Pop: 渲染 Popup（className 包含 alignedClassName 与 options.popupClassName）
Note over Pop: 弹层展示并带有对齐相关类名
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (3)tests/unique.test.tsx (1)
src/context.ts (1)
src/UniqueProvider/index.tsx (1)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @zombieJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves a styling issue within the UniqueProvider component where popups were failing to receive dynamically generated class names based on their alignment. The changes introduce a mechanism to correctly apply these alignment-specific class names, improving the visual consistency and behavior of popups. Additionally, a new prop is provided for greater customization of popup class names based on alignment.
Highlights
- Missing Class Name Fix: The pull request addresses an issue where the
UniqueProvider's popup was not correctly applying alignment-related class names, leading to potential styling inconsistencies. - Dynamic Class Name Generation: New logic has been introduced within
UniqueProviderto dynamically compute class names based on the popup's alignment information, utilizing thegetAlignPopupClassNameutility. - Customizable Alignment Class Names: A new prop,
getPopupClassNameFromAlign, has been added toUniqueShowOptions, allowing developers to provide a custom function to generate additional class names based on the popup's alignment. - Enhanced Test Coverage: A new test case has been added to
tests/unique.test.tsxto verify that thegetPopupClassNameFromAlignprop correctly applies custom class names to theUniqueProvider's popup, ensuring the fix works as expected.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #570 +/- ##
==========================================
+ Coverage 95.83% 95.86% +0.03%
==========================================
Files 18 18
Lines 937 944 +7
Branches 273 276 +3
==========================================
+ Hits 898 905 +7
Misses 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where a unique popup was missing a className. The changes involve adding a getPopupClassNameFromAlign prop and using it within UniqueProvider to compute and apply the correct aligned class name. A new test case is also added to verify this functionality. The implementation is mostly correct, but I've suggested a small refactoring in UniqueProvider/index.tsx to improve the usage of the useMemo hook for better maintainability and adherence to React best practices.
| const alignedClassName = React.useMemo(() => { | ||
| if (!options) { | ||
| return ''; | ||
| } | ||
|
|
||
| const baseClassName = getAlignPopupClassName( | ||
| options.builtinPlacements || {}, | ||
| options.prefixCls || '', | ||
| alignInfo, | ||
| false, // alignPoint is false for UniqueProvider | ||
| ); | ||
|
|
||
| return classNames(baseClassName, options.getPopupClassNameFromAlign?.(alignInfo)); | ||
| }, [ | ||
| alignInfo, | ||
| options?.getPopupClassNameFromAlign, | ||
| options?.builtinPlacements, | ||
| options?.prefixCls, | ||
| ]); |
There was a problem hiding this comment.
The dependency array for React.useMemo uses optional chaining (options?.prop), which is an anti-pattern and can lead to incorrect memoization behavior. The exhaustive-deps lint rule would likely flag this. It's better to depend on the options object directly and destructure the needed properties inside the memoized function. This makes the dependencies clearer and ensures the memoization works as expected when the options object itself changes.
const alignedClassName = React.useMemo(() => {
if (!options) {
return '';
}
const { builtinPlacements, prefixCls, getPopupClassNameFromAlign } = options;
const baseClassName = getAlignPopupClassName(
builtinPlacements || {},
prefixCls || '',
alignInfo,
false, // alignPoint is false for UniqueProvider
);
return classNames(baseClassName, getPopupClassNameFromAlign?.(alignInfo));
}, [alignInfo, options]);
Summary by CodeRabbit